✨ Workflow models: signed bundles, hub marketplace catalogue with nightly refresh, spoke import/publish - #17714

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign hanthor for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Open PR overlap check

Base branch: v5

Conflicts are checked with every PR merged onto the current tip of v5, so a PR that only needs a rebase is not reported as conflicting with its siblings.

No conflicting open PRs or same-file overlaps were found.

Hive status — updated in place; edits do not notify. Full history is in the Activity Monitor.

clubanderson this PR is held at stage/071-needs-author and the hive will not merge it yet.

Why:

• the branch conflicts with the base and needs a rebase

Next: the hive does not push to your branch. Push a new head (or reply here); CI and the review swarm re-run automatically and the PR returns to the normal path.

Review findings from adjudicator-2 (folded here because this head already carries its hive review):

HUMAN DECISION NEEDED — clubanderson should any registered spoke be allowed to publish under the hub's own ingested-unreviewed tier (and squat any name@version permanently) on the public hub?

Security perspective, read at head a305b60 (hub marketplace*.go, hub_keys.go, saas_users_admin.go:requireAdmin, spoke api_workflows_marketplace.go, config.go, the marketplace JS). Auth plumbing is sound: bearer is bound to X-Hive-ID and generation-verified, admin routes go through CSRF + getRealAuthUser, store keys are regex-bound (marketplace_store.go:43-44) so no path traversal, UI output is esc()-wrapped throughout. Two trust-boundary points remain:

1. Spokes can impersonate the hub's crawl tier and permanently squat catalogue keys (medium)

src/pkg/hub/marketplace.go:163-166 lets a non-admin publisher assert ingested-unreviewed:

case bundle.TierLocalPublished, bundle.TierIngestedUnreviewed:

Only the hub's own refresh pipeline should ever produce that tier — it is the label the catalogue uses to mean "the hub crawled this from the CNCF corpus", and marketplace_store.go:286,301 uses it to set newerIngestAvailable on curated entries. Any hive that has heartbeated once (npsHiveRegistered, the whole public spoke population) can therefore publish kubernetes@ingested-20991231.1 with arbitrary model content and (a) show up with the hub-ingest badge, (b) flip the "newer ingest available" flag on the admin-curated entry, (c) pre-empt the real nightly ingest for that day (marketplaceNextIngestVersion just increments past it). The refresh path calls st.Publish directly (marketplace_refresh.go:242), never the HTTP handler, so the handler can refuse this tier for spokes with no loss:

	case bundle.TierLocalPublished:
	case bundle.TierIngestedUnreviewed, bundle.TierCurated, bundle.TierBuiltIn:
		if !who.admin {
			marketplaceJSONError(w, http.StatusForbidden, fmt.Sprintf("tier %q requires a hub admin", b.Tier))
			return
		}

(and drop TierIngestedUnreviewed from the spoke-side passthrough at api_workflows_marketplace.go:406 so it always sends local-published).

Separately, because the store is immutable and the PR adds no delete/unpublish route, any spoke can also squat foo@1.0.0 for any foo — a later admin curated publish gets 409 forever (marketplace_store.go:97). The PR body states immutability as a design goal, so I am flagging the consequence rather than the mechanism: at minimum an admin-only DELETE or a per-name ownership check (first publisher of name owns it, admin overrides) is needed before this is exposed on the public hub.

2. Spoke heartbeat bearer is sent to workflow.marketplace.url on every call, including anonymous GETs (low)

src/pkg/dashboard/api_workflows_marketplace.go:235-237 attaches SpokeHeartbeatKey() unconditionally in hubRequest, and config.go:343-351 lets workflow.marketplace.url point at a host other than hub.url. The index/bundle GETs are unauthenticated on the hub (marketplace.go:34-35), so the credential that lets anyone impersonate this hive to the hub (NPS, selfreport, publish) is handed to whatever host is configured, for no benefit. Suggest attaching the bearer only in handleMarketplacePublish/handleMarketplaceRefresh, and only when url.Parse(base).Host == url.Parse(c.Hub.URL).Host.

Not verified: complexity.ExtractTarGz bounds (pre-existing; used at marketplace_refresh.go:325 with marketplaceMaxTarball).

Review findings from reviewer-2 (folded here because this head already carries its hive review):

docs-currency review (head a305b60)

1. Three new hub env vars are missing from src/docs/env-vars.md (in-scope)

src/pkg/hub/marketplace_refresh.go:36-42 adds HIVE_HUB_MARKETPLACE_EXTRA_REPOS, HIVE_HUB_MARKETPLACE_REFRESH_SCHEDULE, HIVE_HUB_MARKETPLACE_DISABLED (read at :134, :143, :408). src/docs/env-vars.md:620-640 ("Keeping this reference current") requires a row for every new os.Getenv/os.LookupEnv in the same PR and notes CI only checks the opposite direction, so this won't fail a ratchet. The vars are described only in prose in workflow-models.md and hub-server.md, which operators don't consult for env inventory. Suggested rows after HIVE_HUB_DISCORD_REQUESTS_WEBHOOK (env-vars.md:441):

 | `HIVE_HUB_DISCORD_REQUESTS_WEBHOOK` | No | none | Secret Discord webhook URL for the hosted hub's request channel. … |
+| `HIVE_HUB_MARKETPLACE_DISABLED` | No | unset (enabled) | `1`/`true`/`yes` turns the hub workflow-model marketplace off: `/api/marketplace/*` routes return `404` and the nightly refresh goroutine does not start (`src/pkg/hub/marketplace_refresh.go`). |
+| `HIVE_HUB_MARKETPLACE_REFRESH_SCHEDULE` | No | `03:00` when unset | Daily UTC time (`HH:MM`) the hub re-ingests the CNCF corpus into the marketplace. An explicitly empty or invalid value disables the nightly run; on-demand `POST /api/marketplace/refresh` still works. |
+| `HIVE_HUB_MARKETPLACE_EXTRA_REPOS` | No | none | Comma-separated `owner/repo` list ingested alongside the embedded CNCF corpus (`pkg/workflowmodel/ingest/corpus`); entries are lower-cased and de-duplicated. |

2. Docs claim workflow.marketplace.enabled: false hides the Marketplace tab; the UI always renders it (in-scope)

src/docs/design/workflow-models.md:769 says false hides the tab and 404s the routes, and src/pkg/config/config.go:313 says "set false to hide the Marketplace view". But the nav list in src/pkg/dashboard/static-src/js/config-workflows-library.js:277 is static (['marketplace', 'Marketplace'] is always present), and nothing in the diff exposes the flag to the UI — with the setting off, the tab still appears and shows "Could not load the catalogue: workflow marketplace disabled…" (config-workflows-marketplace.js wfMktLoad, which reads the 404 from api_workflows_marketplace.go marketplaceEnabled() guard). Either gate the nav item or, more simply, make the docs match:

    enabled: true          # default true; false 404s the spoke /api/marketplace/* routes (the tab still renders and reports the catalogue as disabled)

and in config.go:313-314: // Enabled defaults to true; set false to return 404 from the proxy routes (the Marketplace view still renders and surfaces the error).

Everything else I checked lines up: GET /api/marketplace/status field list in hub-server.md matches marketplace.go (entries, byTier, etag, generatedAt, corpusRepos, trackedRepos, failingRepos, refreshing, schedule, lastRefresh, lastCheckedAt); tiers listed in the catalogue docs omit local consistently with publish normalising it to local-published; changelog fragment present. Note: no src/docs/api-reference/* page lists the spoke /api/workflows/* routes at base either, so the new spoke routes being only in the design doc + openapi.json is pre-existing, not introduced here.

Review findings from reviewer (folded here because this head already carries its hive review):

Style / conventions review (head a305b60). No blockers from this perspective; two consistency notes worth fixing before merge.

1. workflow.marketplace.refresh_interval breaks the config duration convention — src/pkg/config/config.go:322
Every other operator-facing duration in this package is a string parsed through parseDurationOr (config.go:3368; e.g. StageAdoptionConfig.OfflineGrace/RebaseAttemptTimeout at config.go:3132-3139, Effective* at config.go:3274-3284). This field is a raw time.Duration with a json tag, so the config export / golden renders it as an integer nanosecond count ("refresh_interval": 900000000000), and a bare YAML integer is silently read as nanoseconds. Suggested:

-	RefreshInterval time.Duration `yaml:"refresh_interval,omitempty" json:"refresh_interval,omitempty"`
+	RefreshInterval string `yaml:"refresh_interval,omitempty" json:"refresh_interval,omitempty"`
 }
@@
 func (m MarketplaceConfig) EffectiveRefreshInterval() time.Duration {
-	if m.RefreshInterval <= 0 {
-		return DefaultMarketplaceRefreshInterval
-	}
-	return m.RefreshInterval
+	return parseDurationOr(m.RefreshInterval, DefaultMarketplaceRefreshInterval)
 }

(and RefreshInterval: "1m" in workflow_marketplace_test.go:17).

2. refreshOneRepo encodes three outcomes in one string — src/pkg/hub/marketplace_refresh.go (refreshOneRepo, consumed in runMarketplaceRefresh)
It returns "" for unchanged, the literal "skipped" for skipped, and otherwise a name@version key; the caller then switches on those magic values. A tiny enum keeps the contract explicit:

+type marketplaceRepoOutcome int
+
+const (
+	marketplaceRepoUnchanged marketplaceRepoOutcome = iota
+	marketplaceRepoSkipped
+	marketplaceRepoPublished
+)
+
-func (s *HubServer) refreshOneRepo(ctx context.Context, st *marketplaceStore, rf *marketplaceRefresher, repo string) (string, error) {
+func (s *HubServer) refreshOneRepo(ctx context.Context, st *marketplaceStore, rf *marketplaceRefresher, repo string) (marketplaceRepoOutcome, string, error) {

Minor, no action needed: the doc comment // Run ingests repos (all when nil)… sits on var errMarketplaceRefreshBusy rather than on runMarketplaceRefresh; marketplaceJSONError is a same-signature alias of writeJSONError, which the rest of pkg/hub calls directly; marketplaceHiveIDHeader and short/shortHash are duplicated across dashboard, hub and bundle.

Review findings from adjudicator (folded here because this head already carries its hive review):

Correctness review (head a305b60; read bundle.go, api_workflows_marketplace.go, hub/marketplace{,_store,_refresh}.go and their tests, plus wfstore, ingest.Lifecycle, requireAdmin at merge-base).

1. A transient fetch failure defeats the "unchanged → no-op" invariant and mints duplicate ingested versions (medium)

src/pkg/hub/marketplace_refresh.go:200-204: when rf.fetch fails, the refresh record is overwritten with {Repo, CheckedAt, Error} only — the previous FilesHash/Published/SourceSHA are discarded. On the next successful run, the no-op test at :207 is prev.FilesHash == hash && prev.Error == ""; both halves now fail, so the repo goes through Lifecycle → Export → Publish again and lands a brand-new ingested-YYYYMMDD.1 whose content is byte-identical to the last published version (the hash differs only because Spec.Meta.Version differs, so Publish at marketplace_store.go:70-98 sees it as new). One rate-limit blip or GitHub 5xx on night N therefore produces a duplicate catalogue entry on night N+1 for every affected repo, and newerIngestAvailable is raised on curated entries for nothing (marketplace_store.go:272-276). TestMarketplaceRefreshNoopVsNewVersion (marketplace_test.go:402) only covers a repo that fails on every run, so this path is untested.

Suggested fix — carry the last good state through the error record and let a prior publish (not an empty error) gate the no-op:

--- a/src/pkg/hub/marketplace_refresh.go
+++ b/src/pkg/hub/marketplace_refresh.go
@@ func (s *HubServer) refreshOneRepo(
 	files, sha, err := rf.fetch(fctx, repo)
 	if err != nil {
-		_ = st.saveRefreshRecord(marketplaceRefreshRecord{Repo: repo, CheckedAt: rf.now().UTC(), Error: err.Error()})
+		// Keep the last good hash/publish so a transient failure does not
+		// force a re-ingest (and a duplicate version) on the next run.
+		rec, _ := st.refreshRecord(repo)
+		rec.Repo, rec.CheckedAt, rec.Error = repo, rf.now().UTC(), err.Error()
+		_ = st.saveRefreshRecord(rec)
 		return "", err
 	}
 	hash := marketplaceFilesHash(files)
 	rec := marketplaceRefreshRecord{Repo: repo, SourceSHA: sha, FilesHash: hash, Files: len(files), CheckedAt: rf.now().UTC()}
-	if prev, ok := st.refreshRecord(repo); ok && prev.FilesHash == hash && prev.Error == "" {
+	// Unchanged file set and either already published or legitimately
+	// skipped (no pipelines). A "does not compile" record keeps Published
+	// empty with Error set, so it is retried.
+	if prev, ok := st.refreshRecord(repo); ok && prev.FilesHash == hash && (prev.Published != "" || prev.Error == "") {
 		rec.Published = prev.Published

And a regression test in TestMarketplaceRefreshNoopVsNewVersion: run once (published), make fetch return an error for that repo once, then restore it and assert the third run reports Unchanged with len(res.Published) == 0 and Versions(name) still has one entry.

2. Spoke POST /api/marketplace/refresh can never succeed against the real hub (low, advisory)

api_workflows_marketplace.go:457-470 forwards the refresh with only the heartbeat bearer (hubRequest, :206-209), but the hub route is wrapped in requireAdmin (hub/marketplace.go:39), which resolves a hub session user — a bearer-only request is rejected, so the spoke always answers 403 and the owner-facing Refresh catalogue button in config-workflows-marketplace.js is a dead end. The PR body acknowledges the gate, so this is a UX/dead-code note rather than a blocker; consider hiding the button unless the hub's /api/marketplace/status reports the caller can refresh, or dropping the spoke route until spoke-initiated refresh is actually authorisable.

Not checked: the UI module and openapi additions beyond route parity; the CSRF behaviour of isCSRFSafe for server-to-server POSTs.

Mutation testing

⏱️ The 900s mutation budget ran out, so 3 package(s) were not fully mutated and the results below do not cover them:

• pkg/hub: skipped (not started)
• pkg/workflowmodel/bundle: skipped (not started)
• pkg/workflowmodel/ingest/corpus: skipped (not started)

❌ gremlins did not produce a report for 1 package(s), so no verdict can be derived from this run (see the job log):

• pkg/dashboard: gremlins exited 137

1 mutant(s) in changed files are not reached by any test (NOT COVERED). A maintainer may apply the mutation/acknowledged label to accept this.

Verdict: error (#17349).

Thank you for your contribution! Your PR has been merged.

We'd love to hear how your experience was: share feedback

merged by Hive trusted-author auto-merge: author @clubanderson holds role owner and GitHub permission admin on repo; CI green at a305b60.